feat: generic sidebar notification plugin #1379
Conversation
| > | ||
| <UpgradeNotification {...upgradeNotificationProps} /> | ||
| <UpgradeNotification |
There was a problem hiding this comment.
revert back to what's in main today, no change in behavior if the plugin is off.
| testId="notification-tray-slot" | ||
| pluginProps={{ | ||
| courseId, | ||
| notificationCurrentState: upgradeNotificationCurrentState, |
There was a problem hiding this comment.
This drives the 'red dot' notification icon. I've removed the 'upgrade' from how this is passed to the plugin to keep it general. On the learning MFE side all the variables relating to this are still called 'upgrade...' we should rename all of that eventually. Probably at the same time we remove all the upgrade code from the learning MFE.
I think these would belong on the API for this plugin slot since any plugin here may want to drive that icon. That said, the code for the red dot is tightly coupled to upgrade right now but you could use it creatively for other purposes. It would probably take further consideration on how we handle notifications from multiple sources (if you put more than 1 plugin here) when the need arises. My thinking is the existence of a getter/setter wouldn't need to change so what I have here should be a stable interface.
For better or worse, I think that will be inevitable in some of the first cases (like this one). This will still help guide how to build out an API that we can consider stable, though. |
|
|
||
| return <div data-testid={testId}>{children}</div>; | ||
| }; | ||
| const MockedPluginSlot = ({ children, id }) => ( |
There was a problem hiding this comment.
I had to modify how this works a bit because we get react errors if testId is passed to the PluginSlot component. We can only pass id, pluginProps, and children. I opted to use id instead here since that would be required to have a functional plugin slot.
| @@ -80,7 +80,7 @@ describe('NotificationsWidget', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('renders upgrade card', async () => { | |||
| it('includes notification_widget_plugin slot', async () => { | |||
There was a problem hiding this comment.
for this and other components I split testing the plugin slot and behavior of the default content w/o a plugin since we expect the latter to be removed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/upgrade-plugin-slots #1379 +/- ##
=============================================================
- Coverage 88.30% 88.27% -0.03%
=============================================================
Files 293 293
Lines 5009 5008 -1
Branches 1267 1237 -30
=============================================================
- Hits 4423 4421 -2
- Misses 570 571 +1
Partials 16 16 ☔ View full report in Codecov by Sentry. |
| id="outline_tab" | ||
| pluginProps={upgradeNotificationProps} | ||
| testId="outline-tab-slot" | ||
| id="outline_tab_notifications_plugin" |
There was a problem hiding this comment.
changed the name on these to align convention with existing plugins xxxxx_plugin
zacharis278
force-pushed
the
zhancock/generic-notification-plugin
branch
from
efafc19 to
eba6583
Compare
* feat: add plugin slot for fbe lock paywall (#1347) * feat: Added PluginSlot wrapping UpgradeNotification components (#1366) * chore: Updated PluginSlot mock to support children and test ids * chore: Updated mocked PluginSlot * chore: Added unit test for MockedPluginSlot * fix: Updated slot name ids * feat: Added Plugin Slot wrapping UpgradeNotification in NotificationTray (#1367) * fix: Removed PluginSlot prop scoping for UpgradeNotification (#1369) * feat: generic sidebar notification plugin (#1379) * feat: make notification plugin api generic * feat: include new sidebar and tests * feat: tweak sidebar toggle * style: fix extra space from merge * feat: rename plugin slots --------- Co-authored-by: Alison Langston <[email protected]> Co-authored-by: Zachary Hancock <[email protected]>
This is just a POC to prove out the API of params we're passing I didn't touch the new sidebar or write any tests.Removes upgrade component specific properties from sidebar/notification plugin slots.
Since we're merging into the feature branch this would be the resulting diff against main: master...zhancock/generic-notification-plugin
Risks/Concerns
This component is still highly coupled with the state of the learning MFE that detail is just hidden into the component now. If the redux store changes structure it could break our plugin. Once this is working it would be good to evaluate the component code we've copied over, there's probably a lot of conditions we can carve out if the upgrade component is 2U specific.
Pairs with https://github.com/edx/frontend-plugin-advertisements/pull/10
^^ in case this isn't publicly visible the approach is to useModel and get all of these fields rather than passing them over a pluggable API.
eg